[V2V] Remove fatal nil IP check in preflight check #18496

ghost · 2019-02-27T15:39:28Z

We introduced a check on empty IP address for NICs. However, this should only apply to migrations to OpenStack as it only breaks network port creation. This PR relaxes the check to limit it to migrations toward OpenStack.

We introduced a check on empty IP address for NICs, because it made virt-v2v-wrapper fail when migrating to OpenStack. However, this was not limited to OpenStack and blocked migrations to RHV, so was too strict. Also, virt-v2v-wapper is able to handle NICs without IP address as long as the ip_address field is not part of the mapping we send to it. This PR removes the check on the IP address and removes the ip_address field if it's nil.

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1680487

ghost · 2019-02-27T15:41:37Z

@miq-bot add-label transformation, bug, hammer/yes
@miq-bot add-reviewer agrare, djberg96, jameswnl

miq-bot · 2019-02-27T15:43:13Z

@fdupont-redhat 'agrare, djberg96, jameswnl' is an invalid reviewer, ignoring...

ghost · 2019-02-27T15:47:46Z

@miq-bot add-reviewer agrare

ghost · 2019-02-27T16:08:35Z

@miq-bot add-reviewer djberg96
@miq-bot add-reviewer jameswnl

jameswnl · 2019-02-27T16:21:58Z

spec/models/service_template_transformation_plan_task_spec.rb

@@ -459,7 +459,12 @@

          it "fails if network IP address is nil" do


need to update this as well

jameswnl · 2019-02-27T16:22:45Z

spec/models/service_template_transformation_plan_task_spec.rb

@@ -459,7 +459,12 @@

          it "fails if network IP address is nil" do
            allow(src_nic_2).to receive(:network).and_return(src_network_2b)


can we rename this src_network_2b to be something obvious e.g. nil_ip_network?

Done. Also renamed src_network_2a.

agrare

If this validation is openshift specific it should be extracted to the openshift repo in the openshift subclass

agrare · 2019-02-27T16:42:59Z

app/models/service_template_transformation_plan_task.rb

@@ -256,12 +256,12 @@ def calculate_network_mappings
      source_network = nic.lan
      destination_network = transformation_destination(source_network)
      raise "[#{source.name}] NIC #{nic.device_name} [#{source_network.name}] has no mapping." if destination_network.nil?
-      raise "[#{source.name}] NIC #{nic.device_name} [#{source_network.name}] has an empty IP address." if nic.network.try(:ipaddress).nil?
+      raise "[#{source.name}] NIC #{nic.device_name} [#{source_network.name}] has an empty IP address." if nic.network.try(:ipaddress).nil? && destination_ems.emstype == 'openstack'


👎 really don't like hard coding this into the common base class

The validation is specific to IMS. That's an IMS limitation due to the way CloudForms stores the IP addresses for a single NIC. When a NIC has a number of IPv4 or IPv6 addresses greater than 1, the Network object associated to the Nic object can have a nil IP address.

BTW, we also handle the various providers using send with method names including the provider name. How different is that ? Shoud I split network_mappings into network_mappings_rhevm and network_mappings_openstack ?

@fdupont-redhat Let's create some scaffolding in the provider repo and put it there. I already have ServiceTemplateTransformationPlan at https://github.com/ManageIQ/manageiq-providers-openstack/blob/master/app/models/manageiq/providers/openstack/cloud_manager/service_template_transformation_plan.rb.

So, how about we create ServiceTemplateTransformationPlanTask there as well (in its own file), and then put our rules in there. Let's establish a common interface method. Then have the core method use that.

@bzwei and I looked around yesterday. I think this will "just work" if you subclass it and redefine it in the subclass, as rails should automagically set the type properly. @agrare am I wrong?

@djberg96 exactly as long as you actually have an instance of ManageIQ::Providers::Openstack::CloudManager::ServiceTemplateTransformationPlan it'll call into that method first.

Typically we'd define

def calculate_network_mappings super # call the base code additional_super_awesome_validations_only_for_openstack end

There's some magic happening at https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L341 inside MiqRequest, but I think it will still work.

I would normally call super, but since it's a loop, he might be better off just redefining the method completely, otherwise he'll have to loop again. That, or create some sort of common interface method for a validating check, but that's more work.

Reading virt-v2v-wrapper code, it appears that a missing IP address is not a problem. What fails is a IP address with nil value, as virt-v2v-wrapper will add the None literal to the openstack command. I discussed with Marco and he agreed that there are valid use cases for missing IP address, regardless of BZ#1657126, so we should not forcefully fail but rather prevent virt-v2v-wrapper failure. I updated the PR to reflect that and the network_mappings is again generic.

I've updated the PR title and description.

djberg96 · 2019-03-05T13:27:03Z

app/models/service_template_transformation_plan_task.rb

-        :ip_address  => nic.network.ipaddress
-      }
+        :ip_address  => nic.network.try(:ipaddress)
+      }.delete_if { |_, v| v.nil? }


Hash#compact is simpler. :)

miq-bot · 2019-03-05T14:41:06Z

Checked commits fabiendupont/manageiq@a543ba9~...7d226da with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

…k_for_rhv [V2V] Remove fatal nil IP check in preflight check (cherry picked from commit 35f5025) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686049

simaishi · 2019-03-06T15:42:28Z

Hammer backport details:

$ git log -1
commit 213702bb40ca8a3bc47058dada9c415900412881
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Mar 5 11:42:25 2019 -0500

    Merge pull request #18496 from fdupont-redhat/v2v_relax_empty_ip_check_for_rhv
    
    [V2V] Remove fatal nil IP check in preflight check
    
    (cherry picked from commit 35f50255759c523254c1a6e2d244eb420661c5cf)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686049

miq-bot added bug hammer/yes v2v labels Feb 27, 2019

miq-bot requested a review from agrare February 27, 2019 15:48

miq-bot requested review from djberg96 and jameswnl February 27, 2019 16:09

jameswnl suggested changes Feb 27, 2019

View reviewed changes

agrare self-assigned this Feb 27, 2019

agrare requested changes Feb 27, 2019

View reviewed changes

fabiendupont added 5 commits March 4, 2019 23:26

[V2V] Relax empty IP check when migrating to RHV

a543ba9

Fix rubocop

e269643

Rename network variables in spec. Fix an item description in spec.

0897ce6

Skipping ip_address when nil

2616820

Protect ipaddress call. Remove leftover code.

db92d3c

ghost force-pushed the v2v_relax_empty_ip_check_for_rhv branch from 1b60ae0 to db92d3c Compare March 5, 2019 07:17

Fix rubocop

9e34323

djberg96 reviewed Mar 5, 2019

View reviewed changes

ghost changed the title ~~[V2V] Relax empty IP check when migrating to RHV~~ [V2V] Remove fatal nil IP check in preflight check Mar 5, 2019

Using compact instead of delete_if

7d226da

djberg96 approved these changes Mar 5, 2019

View reviewed changes

agrare approved these changes Mar 5, 2019

View reviewed changes

agrare merged commit 35f5025 into ManageIQ:master Mar 5, 2019

agrare added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 5, 2019

ghost deleted the v2v_relax_empty_ip_check_for_rhv branch March 5, 2019 21:12

simaishi added hammer/backported and removed hammer/yes labels Mar 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Remove fatal nil IP check in preflight check #18496

[V2V] Remove fatal nil IP check in preflight check #18496

ghost commented Feb 27, 2019 •

edited by ghost

Loading

ghost commented Feb 27, 2019

miq-bot commented Feb 27, 2019

ghost commented Feb 27, 2019

ghost commented Feb 27, 2019

jameswnl Feb 27, 2019

ghost Feb 27, 2019

jameswnl Feb 27, 2019

ghost Feb 27, 2019

agrare left a comment

agrare Feb 27, 2019

ghost Feb 27, 2019

djberg96 Feb 27, 2019 •

edited

Loading

djberg96 Mar 1, 2019

agrare Mar 1, 2019

djberg96 Mar 1, 2019

ghost Mar 5, 2019

ghost Mar 5, 2019

djberg96 Mar 5, 2019

ghost Mar 5, 2019

miq-bot commented Mar 5, 2019

simaishi commented Mar 6, 2019

		@@ -459,7 +459,12 @@

		it "fails if network IP address is nil" do

		@@ -459,7 +459,12 @@

		it "fails if network IP address is nil" do
		allow(src_nic_2).to receive(:network).and_return(src_network_2b)

[V2V] Remove fatal nil IP check in preflight check #18496

[V2V] Remove fatal nil IP check in preflight check #18496

Conversation

ghost commented Feb 27, 2019 • edited by ghost Loading

ghost commented Feb 27, 2019

miq-bot commented Feb 27, 2019

ghost commented Feb 27, 2019

ghost commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Mar 5, 2019

simaishi commented Mar 6, 2019

ghost commented Feb 27, 2019 •

edited by ghost

Loading

djberg96 Feb 27, 2019 •

edited

Loading